-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature 219 styling site landing page #221
Feature 219 styling site landing page #221
Conversation
<a class="nav-link" href="{{ url_for('not_implemented') }}">Sign Up</a> | ||
</li> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agenda is not needs to appear in landing page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
@yammesicka do you approve removing existing links?
<a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a> | ||
</li> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('search') }}">Search</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Search is not needs to appear in landing page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
@yammesicka do you approve removing existing links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
W/E you decide
app/templates/base.html
Outdated
<div class="navbar-brand"> | ||
<a href="{{ url_for('home') }}" aria-label="Home"> | ||
<img class="h-8 w-auto sm:h-10" | ||
src="{{ url_for('static', path='/images/icons/calendar-outline.svg') }}" alt="Logo"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use the ionicons element and not the svg file.
<ion-icon name="calendar-outline"></ion-icon>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sure it will be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aviadamar was ionicons checked for performance?
and where are we using popover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the script is in the head as written ?
about the popover, I have no idea, if you find out let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified the scripts to be in the head as stated and it solved the warning in console.
Nevertheless, it stores some items in cache which reduce performance.
I changed the icon in the logo (navbar-brand).
But I didn't manage to find a way to do it in the page's tab icon.
Do you know how to do it in the page's tab icon?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aviadamar - this change done
Please review
<head> | ||
{% block head %} | ||
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
|
||
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" | ||
integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous"> | ||
<!-- Tailwind CSS --> | ||
<link href="https://unpkg.com/[email protected]/dist/tailwind.min.css" rel="stylesheet"> | ||
<link rel="icon" href="{{ url_for('static', path='/images/icons/calendar-outline.svg') }}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link to ionicons and use icons elements and not svg files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, sure, your wish is my command :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aviadamar - I didn't manage to find a way to do it in the page's tab icon.
Do you know how to do it in the page's tab icon?
<a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a> | ||
</li> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure this sections needs to be in landing page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
@yammesicka do you approve removing existing links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you want ^^"
app/templates/index.html
Outdated
</div> | ||
</main> | ||
</div> | ||
<div class="lg:absolute lg:inset-y-0 lg:right-0 lg:w-1/2"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 4 divs that capture only 1 img - maybe it can be simplify ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I've added few of my insights :)
app/static/style.css
Outdated
background: -webkit-linear-gradient(to right, #FAFFD1, #A1FFCE); | ||
background: linear-gradient(to right, #FAFFD1, #A1FFCE); | ||
} | ||
/* body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please eliminate commented out code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment.
I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please review
app/templates/base.html
Outdated
<head> | ||
{% block head %} | ||
<meta charset="UTF-8"> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
|
||
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" | ||
integrity="sha384-giJF6kkoqNQ00vy+HMDP7azOuL0xtbfIcaT9wjKHr8RbDVddVHyTfAAsrekwKmP1" crossorigin="anonymous"> | ||
<!-- Tailwind CSS --> | ||
<link href="https://unpkg.com/[email protected]/dist/tailwind.min.css" rel="stylesheet"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use Tailwind in this project.
We won't add it 'cause it will make a mess of a code (Bootstrap + Tailwind mishmash) and will make the page much heavier than it should be. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issue, I'll remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's almost a valid point, but tailwind
is harder to work with if you don't use React/Vue/Angular, especially for new users, and will create a bloated code which will load slower. ^^"
If I have to choose only one of them (and that's the case) - I take Bootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yammesicka - done - removed tailwind :)
Could you please add visual screenshots in the description? It could be very helpful |
Codecov Report
@@ Coverage Diff @@
## develop #221 +/- ##
===========================================
- Coverage 99.06% 99.02% -0.05%
===========================================
Files 48 49 +1
Lines 2251 2258 +7
===========================================
+ Hits 2230 2236 +6
- Misses 21 22 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good job, Kudos :) As always, added few insights. Please take a look.
app/locales/base.pot
Outdated
@@ -8,7 +8,7 @@ msgid "" | |||
msgstr "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please delete these files? Wev'e changed them so they'll be created only when pushing to main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yammesicka - removed base.pot ; anything else to remove?
<a class="nav-link" href="{{ url_for('agenda') }}">Agenda</a> | ||
</li> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever you want ^^"
<a class="nav-link" href="{{ url_for('view_invitations') }}">Invitations</a> | ||
</li> | ||
<li class="nav-item"> | ||
<a class="nav-link" href="{{ url_for('search') }}">Search</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
W/E you decide
<script src="https://cdn.jsdelivr.net/npm/[email protected]/dist/js/bootstrap.bundle.min.js" | ||
integrity="sha384-ygbV9kiqUc6oa4msXn9868pTtWMgiQaeYH7/t7LECLbyPA2x65Kgf80OJFdroafW" | ||
crossorigin="anonymous"></script> | ||
<!-- This bootstrap version is needed here because of the toggler bug in the beta version--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job commenting about it
app/templates/four_o_four.j2
Outdated
{% block content %} | ||
|
||
<div class="container"> | ||
<h2 class="text-4xl tracking-tight leading-10 font-extrabold text-purple-900 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to adapt from tailwind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/templates/index.html
Outdated
{% block content %} | ||
|
||
<div class="max-w-screen-xl mx-auto"> | ||
<div class="relative z-10 pb-8 bg-white sm:pb-16 md:pb-20 lg:max-w-2xl lg:w-full lg:pb-28 xl:pb-32"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to adapt from tailwind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
app/main.py
Outdated
agenda, calendar, categories, celebrity, currency, dayview, | ||
email, event, invitation, profile, search, telegram, whatsapp | ||
agenda, calendar, categories, celebrity, currency, dayview, email, | ||
event, invitation, profile, search, telegram, whatsapp, four_o_four |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please put in the right place:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Handle issue #219